Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The vm_id coming in from API is a string. #17674

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

lfu
Copy link
Member

@lfu lfu commented Jul 6, 2018

Followup of #17627.

Fixes ManageIQ/manageiq-v2v#471.

@miq-bot assign @gmcculloug
@miq-bot add_label enhancement, transformation

cc @AparnaKarve @bzwei

@AparnaKarve
Copy link
Contributor

LGTM - thanks @lfu !!

@gmcculloug gmcculloug requested a review from bzwei July 6, 2018 20:33
@lfu lfu force-pushed the v2v_pre_post_ansibile_service branch from 6fc4282 to bb86d41 Compare July 6, 2018 20:49
@miq-bot
Copy link
Member

miq-bot commented Jul 6, 2018

Checked commit lfu@bb86d41 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

@AparnaKarve
Copy link
Contributor

AparnaKarve commented Jul 6, 2018

@bzwei @lfu Just so you are aware, this change with vm_id under config_info.actions is going to impact our frontend code in a big way.

Before -

"options": {
  "config_info": {
  "transformation_mapping_id": "3",
  "vm_ids": [
    "1",
    "2"
    ]
  }
}

After -

"options": {
  "config_info": {
  "transformation_mapping_id": "3",
  "actions": [
  {
      "vm_id": "4",
      "pre_service": false,
      "post_service": false
   }
  ]
 }
}

We have a couple of places in the code where we reference vm_ids directly from config_info -- all those places would now have to be rectified.
Not to mention, none of our existing Plans are going to work because of the above change.

Just wanted to check if it would be possible to re-introduce the vm_ids array under config_info?
Or the expectation going forward is that the old plans are not supposed to work?

Also, would this change be backported to Gaprindashvili?

/cc @priley86

@bzwei
Copy link
Contributor

bzwei commented Jul 6, 2018

v2v feature is not officially released. I don't think we have to maintain backward compatibility. Please ping PM if it is a concern.

Sorry it requires more front end code changes, but it is not reasonable to keep vm ids and playbooks separate.

@bzwei
Copy link
Contributor

bzwei commented Jul 6, 2018

BTW, you can omit

      "pre_service": false,
      "post_service": false

if the value is false.

@gmcculloug
Copy link
Member

@AparnaKarve Are you ok with @bzwei response here or do we need to discuss?

@AparnaKarve
Copy link
Contributor

@gmcculloug I think we can merge this.

Would this be backported to Gaprindashvili?

@AparnaKarve
Copy link
Contributor

Just got the confirmation that we are planning on supporting pre and post playbook in 5.9.4, which means, this PR and #17627 should be gaprindashvili/yes.
Thanks.

@JPrause
Copy link
Member

JPrause commented Jul 9, 2018

@miq-bot add_label blocker

@miq-bot miq-bot added the blocker label Jul 9, 2018
@JPrause
Copy link
Member

JPrause commented Jul 9, 2018

If this can be backported, please add the gaprindashvili/yes label

@AparnaKarve
Copy link
Contributor

@miq-bot add_label gaprindashvili/yes

@gmcculloug gmcculloug merged commit 1bb13c5 into ManageIQ:master Jul 9, 2018
@gmcculloug gmcculloug added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 9, 2018
simaishi pushed a commit that referenced this pull request Jul 25, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit c217a7b63a89deadc6abddad450d2bd380200dab
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Jul 9 16:10:19 2018 -0400

    Merge pull request #17674 from lfu/v2v_pre_post_ansibile_service
    
    The vm_id coming in from API is a string.
    (cherry picked from commit 1bb13c5e8d627d50b9cede9505c43392f11f3bec)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1608420

@lfu lfu deleted the v2v_pre_post_ansibile_service branch September 29, 2018 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants